feat: pre-warm drill targets — no empty-view flash on drill push/pop - #160
Conversation
Drilling deploy -> rs -> pods visibly happened in two steps: the pane switched to a just-cleared bucket (WatchManager.start clears before the LIST), rendered empty, and filled one network RTT later. Esc had the same shape on the way back (the parent watch was stopped on the way down, so navigating back re-cleared + re-LISTed). _drill_into and _pop_drill now warm the target first: start the watch while the current view is still up (a kind no pane displays renders nowhere), wait - bounded by DRILL_PREWARM_TIMEOUT - until the rows the transition will show exist (owned_by(parent_uid) for a push, any parent row for a pop), then run the unchanged push/pop+navigate transaction. _navigate_locked's start() is a no-op by then, the bucket is warm, and the single post-switch render lands with real rows. While waiting the status bar carries 'loading <kind>' - the corvid busy indicator (#143) animates it. - a live watch (split pane) skips both restart and wait - timeout degrades to the old switch-then-fill, never worse - _stop_watch_if_unused reaps the pre-warmed stream when the drill lost its pane or raced a scope change - NavigationStack.peek() for the pop-side prewarm target test_concurrent_drill_and_navigate_stay_consistent now gates on the drill actually blocking inside the critical section instead of a sleep - the prewarm shifted the old timing assumption; the invariant it pins is unchanged. Closes #157 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Pre-warms drill targets so push/pop transitions render populated views without empty-state flashes.
Changes:
- Adds bounded watch pre-warming with progress feedback and cleanup.
- Adds non-mutating navigation stack inspection.
- Adds push, pop, timeout, progress, and split-pane tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/korvid/ui/app.py |
Implements drill target pre-warming. |
src/korvid/ui/navigation.py |
Adds NavigationStack.peek(). |
tests/ui/test_drilldown.py |
Tests pre-warm behavior and rendering. |
Suppressed comments (1)
src/korvid/ui/app.py:2301
- The stack top is not guaranteed to still be
peeked: pre-warming happens outside_nav_lock, so a later agent drill can push and navigate while this Escape is waiting. This code then pops that newly added level and navigates back, allowing the older pop to undo the newer action. Verify the top is still the captured level under the lock, or restart/abort the stale pop before mutating the stack.
popped = pane.drill.pop()
if popped is None:
return False
await self._navigate_locked(pane, popped.parent_kind, None)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review round 1 on #160: the pre-warm widened the window between the drill's intent and its locked transaction to up to a second, so a newer :view/:ns or a context switch landing meanwhile could be overridden by the stale drill (or worse, an old-cluster UID applied after the epoch changed). Both _drill_into and _pop_drill now anchor (kind, scope) and _ctx_epoch before the pre-warm and revalidate under the lock - a mismatch abandons the drill with an accurate result string (never a false 'drilled into ...' success for the agent), and the pop side also requires the peeked level to still be the top of the stack. Abandoned streams are reaped by the existing _stop_watch_if_unused finally. Tests: test_drill_abandons_when_a_newer_navigation_lands_during_prewarm, test_drill_abandons_across_a_context_epoch_change, test_pop_abandons_when_the_view_changed_during_prewarm. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/korvid/ui/app.py:2274
- This stale check misses a newer explicit navigation to the same
(kind, scope).on_navigate_commandstill represents a later command and clears drill state, but because the tuple and context epoch remain unchanged, the older prewarm then pushes its level and overrides that command. Track and revalidate a per-pane navigation/request generation that advances even for same-target navigations.
if (
(pane.kind, pane.scope) != origin
or self._ctx_switching
or epoch != self._ctx_epoch
):
src/korvid/ui/app.py:2269
- Returning
Nonedenotes success toagent_drill_down, so if the initiating pane is closed during the newly added prewarm window the agent reports that it drilled and records a breadcrumb even though no navigation occurred. Return an abandonment error here so callers receive an accurate outcome.
if pane not in self._panes:
return None # the initiating pane was closed while queued
src/korvid/ui/app.py:2201
activealso includes a watch started by another in-flight prewarm, not only a watch serving a pane. If an agent drill and keyboard drill overlap, the second call returns here before the LIST lands, switches to the target, and recreates the empty-view flash. Cleanup can also stop that shared watch when one drill abandons. Distinguish pane-backed watches from prewarm leases (for example, wait on each caller's readiness predicate and reference-count leases so only the last unused lease stops the watch).
This issue also appears in the following locations of the same file:
- line 2268
- line 2270
if (kind, scope) in self.watch_manager.active:
return
await self.watch_manager.start(kind, scope)
Review round 2 on #160 (all three suppressed findings credible): - a ':view deployments' issued while already on deployments is still the newer command (it clears drill state) but left the (kind, scope) tuple and epoch unchanged - the stale drill pushed over it. Every _navigate_locked call now advances a per-pane nav_gen, and both drill paths capture + revalidate it under the lock (test_drill_abandons_when_a_same_target_navigation_lands_during_prewarm). - a pane closed during the pre-warm returned None, which agent_drill_down reports as a successful drill with a breadcrumb - now an accurate abandonment string (test_pane_closed_during_prewarm_reports_abandonment). - watch_manager.active includes another drill's in-flight pre-warm, so an overlapping drill skipped its wait and recreated the empty flash, and one drill's cleanup could stop the stream the other relied on. _prewarm_view skips only pane-backed watches, always waits on the caller's own readiness, and _stop_watch_if_unused is lease-counted: only the last release may reap an undisplayed stream (test_overlapping_drills_do_not_skip_each_others_prewarm). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Round 2's three suppressed findings were all credible and are fixed:
Full gate green (2952 passed). |
There was a problem hiding this comment.
이슈 #157 드릴 프리웜 리뷰. 설계가 탄탄합니다: (1) 스위치 전에 타깃 watch를 시작해 LIST가 보이지 않는 곳에서 수행되고, 렌더 스파이 테스트가 (kind, 0) 렌더가 한 번도 없음을 직접 고정. (2) 프리웜 대기(최대 1s)로 넓어진 경합 창을 origin+nav_gen+_ctx_switching+epoch 4중 앵커로 락 안에서 재검증 — 특히 같은 (kind,scope)로의 :view 재진입까지 잡는 per-pane nav_gen 카운터와 그 전용 테스트가 좋습니다. (3) pop 쪽은 peek() identity 비교까지 더해 stale pop이 네비게이션을 덮어쓰지 않고 Esc만 소비(True). (4) _prewarm_leases 카운트로 겹치는 드릴이 서로의 프리웜을 warm으로 오인하지 않게 하고(재현 테스트 포함), finally의 _stop_watch_if_unused가 마지막 lease에서만 pane 미표시 스트림을 회수. (5) pane 닫힘 시 None 대신 정확한 abandonment 문자열을 반환해 agent_drill_down의 거짓 성공 보고를 막은 것, 기존 동시성 테스트의 20ms sleep을 entered 이벤트 게이트로 바꾼 것 모두 올바른 방향입니다. 타임아웃 시 기존 switch-then-fill로 정확히 퇴행하는 것도 확인. 인라인으로 예외/취소 경로의 lease·watch 누수 가능성 1건만 남깁니다 (정상 경로에는 영향 없음).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/korvid/ui/app.py:2222
- The pane-backed fast path races watch teardown.
_navigate_lockedleavespane.kind/scopeunchanged while awaitingWatchManager.stop(), butstop()removes the key fromactivebefore that await. A concurrent drill can therefore see the target pane here and skip bothstart()and the wait; once the other navigation completes, the target watch is gone, so the drill's laterstart()clears the bucket and the empty-view flash returns. The lease also is not consulted by_navigate_lockedor pane-close watch stops. Please synchronize this check with watch lifecycle, or require an active watch and make teardown honor outstanding pre-warm leases.
self._prewarm_leases[key] = self._prewarm_leases.get(key, 0) + 1
if any((p.kind, p.scope) == key for p in self._panes):
return
src/korvid/ui/app.py:2348
- This readiness test is insufficient when popping the second level of
deployments → replicasets → pods. After the pop, the remaining drill level filters replicasets by the deployment UID (_render_panelines 1297-1299), butbool(rows)can become true for an unrelated ReplicaSet first; the switch then renders zero visible rows and flashes empty until an owned row arrives. Build the readiness predicate from a copied stack after one pop, and wait for a row owned by its remainingparent_uid(falling back tobool(rows)only when the pop returns to the root).
await self._prewarm_view(peeked.parent_kind, prewarm_scope, lambda rows: bool(rows))
Review round 3 on #160: - the lease acquire sat outside the try: a drill task cancelled mid-pre-warm (:ctx teardown) left a permanent lease - blocking every later reap on that (kind, scope) - and leaked the started watch. The pre-warm call moved inside the try; the acquire is synchronous before the first await, so the finally never releases a lease that was not taken (test_cancelled_prewarm_releases_its_lease_and_watch). - the pane-backed fast path raced watch teardown: a pane mid-navigate keeps its tuple while stop() has already removed the stream. The fast path now also requires the watch to be live, and _navigate_locked's teardown skips streams with outstanding pre-warm leases - the last lease release reaps them (test_prewarm_restarts_a_dead_watch_even_when_pane_backed, test_navigation_teardown_honors_outstanding_prewarm_leases). - popping pods -> replicasets keeps the deployment-UID filter, but the readiness accepted any row: an unrelated ReplicaSet arriving first switched into a zero-row filtered view. Readiness now mirrors what the post-pop view will show - owned rows for a remaining level, any row only at the root (test_two_level_pop_waits_for_rows_the_drill_filter_will_show). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/korvid/ui/app.py:2224
- This lease is not honored by
_close_focused_pane, which unconditionally stops the closing pane's distinct watch atapp.py:4498-4499. If a drill targets a live split pane while that pane is being closed (the close can yield while shutting down its log pane), pre-warm observes the still-active rows and returns, then the close kills the leased watch;_navigate_lockedrestarts it and clears the bucket, recreating the empty flash. Make pane-close teardown use the same lease check as navigation, leaving the final lease release to reap the watch.
self._prewarm_leases[key] = self._prewarm_leases.get(key, 0) + 1
There was a problem hiding this comment.
재리뷰 (신규 커밋 1개, cc144e6 → f1b4ad4) — 라운드 3의 inline Warning이 정확히 해결되었고, 추가로 두 가지 레이스까지 정리되었습니다.
1. 리스 누수 해결 (이전 Warning) — _prewarm_view await가 try/finally 안으로 이동했습니다. 리스 획득은 첫 await 이전의 동기 구간이므로 "획득하지 않은 리스를 finally가 해제하는" 역누수도 구조적으로 불가능합니다. test_cancelled_prewarm_releases_its_lease_and_watch가 취소 후 리스 소멸 + watch 미잔존을 실제로 검증합니다.
2. pane-backed fast path의 teardown 레이스 — 판이 튜플을 유지한 채 stream이 이미 stop()된 경우를 key in self.watch_manager.active 조건 추가로 막았고, 대칭으로 _navigate_locked의 teardown이 outstanding 리스가 있는 stream을 건너뛰도록 하여(마지막 리스 해제가 reap) fast path와 teardown 양쪽이 일관됩니다. 두 방향 모두 테스트로 고정 (test_prewarm_restarts_a_dead_watch_even_when_pane_backed, test_navigation_teardown_honors_outstanding_prewarm_leases).
3. 2단계 pop의 readiness 정합성 — pods→replicasets pop은 deployment-UID 필터를 유지하는데, 기존 readiness는 임의 행을 수락해 무관한 ReplicaSet이 먼저 도착하면 0행 필터 뷰로 전환될 수 있었습니다. 이제 under = pane.drill.copy(); under.pop()으로 pop 이후 뷰가 실제로 보여줄 조건(잔여 레벨이면 owned 행, 루트면 임의 행)을 그대로 미러링합니다. 스파이 렌더 테스트가 ("replicasets", 0) 렌더 부재를 고정합니다.
지적사항 없음. APPROVE
Closes #157
Problem
Drilling
deploy → rs → pods(and Esc back) read as two visible steps per transition: jump to an empty child view, then rows pop in a beat later. Root cause:_drill_intoswitched the pane first;WatchManager.startthen cleared the bucket and began the LIST, so the first render of the child view was always against an empty store, and content arrived one network RTT later._pop_drillhad the same shape (the parent watch was stopped on the way down → re-clear + re-LIST on the way back).Fix — pre-warm the target view before switching
_drill_into/_pop_drill(and everything routed through them: Enter, Esc,agent_drill_down, helmhhistory) now:DRILL_PREWARM_TIMEOUT(1s), until the rows the transition will show exist —owned_by(parent_uid)rows for a push, any parent row for a pop. All three drill chains qualify (replicasets,pods, and helmhelmrevisionsall carryowner_uids).loading <kind>on the status bar — the corvid busy indicator (Corvid-themed busy indicator: animated "bird at work" while long operations run (helm install, previews, drains) #143) animates it, so the wait reads as working._navigate_locked'sstart()is a no-op by then (watch already running → bucket not re-cleared), and the single post-switch render lands with real rows.Edge handling
_stop_watch_if_unusedruns in afinallyafter the transaction — a drill whose pane closed while queued (or that raced a scope change) never leaks a stream; a landed navigation makes it a no-op.NavigationStack.peek()added for the pop-side prewarm target; a pop that races a concurrent pop stops the wrongly-warmed watch via the same reaper.Testing
New in
tests/ui/test_drilldown.py(RED-first against a source with simulated LIST RTT, render spy asserts no(kind, 0)render ever happens):pilot.presswould await the whole transition)until()polling only)test_concurrent_drill_and_navigate_stay_consistentnow gates on the drill actually being blocked inside the critical section (event set in the stop() seam) instead of a 20ms sleep — the prewarm shifted the old timing assumption; the invariant it pins (no stranded drill state) is unchanged and still asserted.Full gate green: ruff, mypy --strict, tach, 2946 passed / 21 skipped, coverage ≥ 80%.